Skip to content

Fix REPL crash when V8 emits warnings during preview mode (Issue #63473)#63491

Open
DivyanshuX9 wants to merge 1 commit into
nodejs:mainfrom
DivyanshuX9:fix/issue-63473-repl-asm-warning
Open

Fix REPL crash when V8 emits warnings during preview mode (Issue #63473)#63491
DivyanshuX9 wants to merge 1 commit into
nodejs:mainfrom
DivyanshuX9:fix/issue-63473-repl-asm-warning

Conversation

@DivyanshuX9
Copy link
Copy Markdown

Fix: Handle V8 Warnings in DisallowJavascriptExecutionScope

Fixes: #63473


The Problem

When the Node.js REPL runs with preview mode enabled, certain code (such as
asm.js modules) causes V8 to emit deprecation warnings inside a
DisallowJavascriptExecutionScope : a V8 context where JavaScript execution
is temporarily forbidden.

Node's warning handler (PerIsolateMessageListener) was unconditionally
calling ProcessEmitWarningGeneric, which invokes process.emit() -> a JS
call which is inside this forbidden scope, causing a fatal crash:

Fatal error in , line 0

Invoke in DisallowJavascriptExecutionScope


What Changed

Modified PerIsolateMessageListener in src/node_errors.cc to check
can_call_into_js() before attempting to emit warnings.

When JavaScript cannot be called (i.e. we are inside a forbidden scope),
the warning is deferred via env->SetImmediate() until after the scope
exits at which point it is safe to call JavaScript again.

Files changed:

  • src/node_errors.cc -> added can_call_into_js() guard with deferred emission
  • test/parallel/test-repl-asm-warning-no-crash.js : new regression test

What Is Preserved

The fix does not degrade warning quality in any case:

Property Status
Full (node:PID) [DEPXXXX] DeprecationWarning: format Preserved
--redirect-warnings file routing Preserved
All existing warning behaviour in normal (non-forbidden) scopes Unchanged
No new allocations or memory leak risk Lambda captures string by value

Approach

Instead of falling back to raw fprintf(stderr, ...) (which loses structured
formatting and --redirect-warnings support), this fix uses env->SetImmediate()
to queue the warning for the next safe event loop iteration.

This is an established Node.js pattern for deferring work that requires
JavaScript execution. The lambda captures the warning string by value,
ensuring it remains valid until the callback fires.

// Before (crashes inside DisallowJavascriptExecutionScope)
ProcessEmitWarningGeneric(env, message, ...);

// After (defers safely when JS is forbidden)
if (!env->can_call_into_js()) {
  std::string deferred_msg = message;
  env->SetImmediate([deferred_msg](Environment* env) {
    ProcessEmitWarningGeneric(env, deferred_msg, ...);
  });
  return;
}
ProcessEmitWarningGeneric(env, message, ...);

How to Test

Run the regression test in isolation:

node test/parallel/test-repl-asm-warning-no-crash.js

Or as part of the test suite:

npm test -- test/parallel/test-repl-asm-warning-no-crash.js

Regression Test

test/parallel/test-repl-asm-warning-no-crash.js reproduces the issue by:

  1. Starting a Node.js REPL with usePreview: true (triggers the inspector path)
  2. Sending the asm.js reproduction case from V8 warnings crash process within DisallowJavascriptExecutionScope #63473
  3. Asserting the process exits cleanly with exit code 0
  4. Asserting no FATAL ERROR appears in stderr

Checklist

  • Regression test added
  • No breaking changes to existing behaviour or public API
  • --redirect-warnings routing preserved
  • No changes outside the two files listed above
  • Commit message follows Node.js format
  • No trailing whitespace or formatting changes to unrelated code

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 22, 2026
@DivyanshuX9
Copy link
Copy Markdown
Author

@Renegade334 could you please review this fix?

Copy link
Copy Markdown
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this and #63486 seem to have borrowed from each other, but the core concept seems to have originated here, so I'm going to aim to get this one merged.

You need to fix your commit authorship information, and add a Signed-off-by: trailer as per

6. Your commit must contain the `Signed-off-by` line with your name and email
address as an acknowledgement that you agree to the [Developer Certificate of Origin][].
Bot generated commits are exempt from this requirement. If a commit has
multiple authors, the `Signed-off-by` line should be added for each author;
and at least one should match the author information in the commit metadata.
This rule does not apply to dependency updates (e.g. cherry-picks), release
commits, or backport commits.

Comment thread src/node_errors.cc Outdated
Comment thread src/node_errors.cc Outdated
Comment thread test/parallel/test-repl-asm-warning-no-crash.js Outdated
@DivyanshuX9 DivyanshuX9 force-pushed the fix/issue-63473-repl-asm-warning branch from 0d433ed to c1c87a7 Compare May 24, 2026 09:30
@DivyanshuX9
Copy link
Copy Markdown
Author

Removed the obsolete REPL asm warning regression test and updated node_errors.cc so deferred warnings capture warning directly in SetImmediate instead of copying to warning_str. Also added the Signed-off-by trailer and pushed the rewritten branch history.

@Renegade334 please check again

@DivyanshuX9 DivyanshuX9 requested a review from Renegade334 May 24, 2026 09:33
Defer non-critical warnings to the next event loop iteration when
can_call_into_js() returns false. This prevents crashes when V8
emits warnings during REPL preview evaluation or other contexts
where JavaScript execution is temporarily forbidden.

When a warning is emitted inside DisallowJavascriptExecutionScope,
ProcessEmitWarningGeneric cannot be called immediately. Instead,
use env->SetImmediate() to queue the warning emission for after
the scope exits. This preserves full warning formatting, deprecation
codes, and --redirect-warnings routing.

Fixes: nodejs#63473
Signed-off-by: Divyanshu Sharma <Divyanshu88999@gmail.com>
@DivyanshuX9 DivyanshuX9 force-pushed the fix/issue-63473-repl-asm-warning branch from c1c87a7 to 99b20d4 Compare May 24, 2026 09:41
Copy link
Copy Markdown
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V8 warnings crash process within DisallowJavascriptExecutionScope

4 participants